-
-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generic var templates for operators and std::iterator_trait var/fvar specialization #1525
Conversation
…rator templates use requires_ to take in more generic value types
…xp1~20180509124008.99 (branches/release_50)
…xp1~20180509124008.99 (branches/release_50)
…stable/2017-11-14)
…ath into cleanup/generic-templates-var
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98) |
@bob-carpenter ready for review! |
@SteveBronder Should I really be reviewing this given that I wrote some of it? |
Good point! Would anyone else mind taking a look at this? |
OK. I'll just go ahead and review today or tomorrow---it will have two of us looking at all the code. This is going to take some reviewing and I have to head out soon, so I can get to it tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read through this mainly because I happened to see how you dealt with std::isinf(), which hit me in #1545 (and I had worked around it in not such a nice way). It turned out that I think I've learned a lot of new things, so it was time well spent.
As you'll see, most of comments concern the ordering of typenames that doesn't match the ordering of function arguments. Of course that's not a big deal, so I leave it up to you what do about that.
I totally skipped stan/math/rev/core/std_iterator_traits.hpp
because I can't even pretend to know what use it has, and also stan/math/rev/scal/fun/pow.hpp
because I haven't cracked std::forward
yet. So you should get a second pair of more experienced eyes on this! :)
Glad to hear and thanks for looking this over!
I think that's v reasonable to fix up
That's fair, after snooping around online
Edit: Eh that seems like more than we need |
Worst case we had 3 sets of eyes on it now (thanks again @mcol!) so I think that's fine |
(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.01) |
@@ -80,16 +83,22 @@ class precomputed_gradients_vari : public vari { | |||
* specified value, vector of operands, and vector of partial | |||
* derivatives of value with respect to the operands. | |||
* | |||
* @tparam Arith An arithmetic type | |||
* @tparam VecVar A vector of vars | |||
* @tparam VecArith A vector of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My requests mainly fall into three broad categories:
-
break each function into its own file and document everything
-
replace comparisons and logical operators with single definition in
prim
usingvalue_of_rec
-
explain why we have
Var
template parameters combined withrequire_var_t<V>
rather than justvar
in all the overloads
I think this would've been easier broken into two PRs, one with the updated metaprograms and one with updated operators, but I'm OK keeping it as is given that the review's in progress.
stan/math/rev/core/cmath.hpp
Outdated
@@ -0,0 +1,119 @@ | |||
#ifndef STAN_MATH_REV_CORE_CMATH_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a few things from <cmath>
---the rest of the functions defined in that header follow the Stan convention of being broken into their own files. I don't think we want to stray from that convention here. Sorry for all the extra work---it's one of the reasons I was dreading breaking this all out into tests and definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at cmath it looks like all these are in the cmath header? For clarification you want signbit
, isinf
, isfinite
, isnan
, copysign
, and isnormal
to be in their own header files?
stan/math/rev/core/cmath.hpp
Outdated
template <typename T, require_autodiff_t<T>...> | ||
inline bool signbit(T&& v) { | ||
using std::signbit; | ||
return signbit(v.val()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write this with value_of_rec(v)
and make it generic across our autodiff types? Or havit recurse with v.val()
if both forward-mode and reverse-mode support .val()
. Either way, it can go in prim
as it's not mentioning any autodiff types itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah since fvar and var have .val()
we can move this to prim
. Should we also have each of these for arithmetic types or just rely on the cmath impl for those (in case someone calls stan::math::signbit(a)
where a is double)?
If we wanted to be v smart we could break these up to have signatures for
- Arithmetic types
- AD types with an arithmetic return from
.val()
- AD types with heavier types in
.val()
(i.e.complex<>
/fvar<var>
/fvar<fvar<var>>
)
Then 1 and 2 would pass by copy while 3 would be able to forward along / pass a reference to .val()
@@ -473,24 +473,22 @@ using require_any_not_fvar_t | |||
= require_any_not_t<is_fvar<std::decay_t<Types>>...>; | |||
|
|||
template <typename T> | |||
using require_var_or_fvar_t = require_t<is_var_or_fvar<T>>; | |||
using require_autodiff_t = require_t<is_autodiff<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[comment]
These are all super useful. Thanks for adding.
|
||
template <typename T> | ||
using require_not_var_or_fvar_t = require_not_t<is_var_or_fvar<T>>; | ||
using require_not_autodiff_t = require_not_t<is_autodiff<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly prefer
-
removing the
require_
prefix because that's implying a use, not a behavior -
replace
_t
with_v
because it denotes a boolean value, not a type; this follows the behavior ofis_arithmetic_v
;enable_if_t
ends in_t
because it denotes a type (void
by default), not a value.
For example, that'd be not_autodiff_v<T>
for true
if T
is not an autodiff type, and usage would be, for example enable_if_t<not_autodiff_v<T>, U>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing the
require_
prefix because that's implying a use, not a behavior
Can you clarify? I'm not sure i understand how the require part of the name is implying a use over a behaviour. The intent with the require_
in the name is because all those require_*
s are trying to emulate a legacy version of C++20 concepts. It's for if you want a universal reference in the function signature for all the weird value types and const combinations. In terms of use/behavior I think my answer is that I think about C++ concepts like compile time contracts (which feels like a behaviour, though idk what that word means in context here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace _t with _v because it denotes a boolean value, not a type; this follows the behavior of is_arithmetic_v; enable_if_t ends in _t because it denotes a type (void by default), not a value.
For example, that'd be not_autodiff_v for true if T is not an autodiff type, and usage would be, for example enable_if_t<not_autodiff_v, U>.
The require_*_t
metaprogramming stuff is essentially a convoluted / compact way to write std::enable_if_t<...>
i.e. they also return void
.
You can think of require_autodiff_t
as an alias for enable_if_t<not_autodiff_v<T>, U>
.
Also idt we can use C++ compile time constexpr objects with c++1y (anything _v like how std does) since the windows compiler we require compatibility with for Rtools has a bug for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood and thought this was a boolean. Now that I see what it's doing, I still think it needs to be renamed. How about enable_if_autodiff_t
to enable if it's an autodiff type? That follows the standard library naming conventions more closely than require_autodiff_t
.
We can then use disable_if_autodiff_t
instead of enable_if_not_autodiff_t
.
We can build similar ones for primitives, enable_if_arithmetic_t
and disable_if_arithmetic_t
.
|
||
template <typename... Types> | ||
using require_all_var_or_fvar_t = require_all_t<is_var_or_fvar<Types>...>; | ||
using require_all_autodiff_t = require_all_t<is_autodiff<Types>...>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these all have doc or are they so close to the ones without _v
that they're obvious? cppreference actually doesn't doc these and they doc everything else meticulously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been throwing this back and forth with myself. I've leaned towards not doc since aliases are inlined in the docs you can see the full definition
http://mc-stan.org/math/d0/dfb/group__require__base__types.html#gac55eae5c1fc2f38a96336b5f0bc94449
* @param a Argument variable. | ||
* @return Negation of variable. | ||
*/ | ||
inline var operator-(const var& a) { | ||
template <typename Var, require_var_t<Var>...> | ||
inline var operator-(Var&& a) { | ||
return var(new internal::neg_vari(a.vi_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional as it wasn't in your code]
Just wanted to throw this out for the future. When we don't return refs, we can just use the braces constructor:
return { new internal::neg_vari(a.vi_) };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
* @param[in] x argument | ||
* @return negation of argument value | ||
*/ | ||
inline bool operator!(const var& x) { return !x.val(); } | ||
template <typename Var, require_var_t<Var>...> | ||
inline bool operator!(Var&& x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there are no derivatives, this can also be done as a single primitive template using value_of_rec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I think with all these it would be good to do them as a seperate PR else this is going to blast off into like a 800-1000 line change PR
stan/math/rev/core/std_isinf.hpp
Outdated
@@ -13,7 +13,8 @@ namespace std { | |||
* @param a Argument. | |||
* @return 1 if argument is infinite and 0 otherwise. | |||
*/ | |||
inline int isinf(const stan::math::var& a) { | |||
template <typename Var, stan::require_var_t<Var>...> | |||
inline bool isinf(Var&& a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be made fully generic in prim
.
@@ -257,7 +260,7 @@ class gp_periodic_cov_vari<T_x, double, T_l, T_p> : public vari { | |||
sin_2_dist_[pos] = sin(2.0 * pi_div_p * dist); | |||
sin_dist_sq_[pos] = sin_dist_sq; | |||
cov_lower_[pos] = new vari( | |||
sigma_sq_d_ * std::exp(sin_dist_sq * neg_two_inv_l_sq), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK weither way with this, but if everything's guaranteed to be int
or double
, there's no reason not to directly qualify the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like there's a reason i did this but I can't remember. I'll change it back and if something breaks we'll know what's up
stan/math/rev/scal/fun/pow.hpp
Outdated
} | ||
return var(new internal::pow_vd_vari(base.vi_, exponent)); | ||
return var( | ||
new internal::pow_vd_vari(base.vi_, static_cast<double>(exponent))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though you're allowing exponent
to be int
now, I don't think you need the static cast to double if the only constructor available for pow_vd_vari
does it for you.
Hmm, that's a really weird error I'll look into what's happening here tmrw |
Actually the only way I know how to fix this
Is to make the |
…gin' into cleanup/generic-templates-var
…ath into cleanup/generic-templates-var
…stable/2017-11-14)
Actually Bob, kind of a wild take but should we pass all these vars by value? They are 8 bytes and like a double can just fit in a word** ** on 64 bit systems which I think is our main target anyway |
Uh, passing by value seems very good Running performance-tests-cmdstan with
|
We should be able to pass var by value. I assume 1.05 means it's faster? What's this testing?
|
This is just running the basic cmdstan performance tests that get kicked off with every PR, but locally on my home computer |
…up/generic-templates-var
I brought this up to date with develop. I think the last failure upstream was some kind of CI failure, not a problem with this PR. |
…up/generic-templates-var
@SteveBronder : There's now an ambiguity in
All of these require one https://en.cppreference.com/w/cpp/numeric/math/pow It seems to be on my machine because I'm not getting the ambiguity. The ambiguity arose in this PR because the Would it be possible to replace the We might also consider adding our own |
If you don't have time for this, do you mind if I cherry pick the bits I need for |
I tried the fully templating |
The problem now is that the test server is running out of memory. @serban-nicusor-catena ---should we just restart the tests or does something else need to happen?
|
Argh, Nic did apply this fix https://www.intel.com/content/www/us/en/programmable/support/support-resources/knowledge-base/embedded/2016/cc1plus-exe--out-of-memory-allocating-65536-bytes.html but it seems to not do the trick. It only occurs occasionally on the recently added Windows machine (running in our lab in Ljubljana). Let me know @serban-nicusor-toptal if I can do something you cant remotely. Will also buy some additional RAM. I restarted the upstream tests https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-1525/30/pipeline |
Thanks for the help here @rok-cesnovar |
Yatta! The fully-templated version works. This is useful as I'll be able to do this elsewhere I've been having trouble with unwanted promotion of primitives to @SteveBronder or @rok-cesnovar : would one of you review my changes to I approved the changes I was waiting for. |
Sorry for the delay I'll take a look at this today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with one small comment
inline var pow(const var& base, Arith exponent) { | ||
template <typename Var, typename Arith, require_var_t<Var>..., | ||
require_arithmetic_t<Arith>...> | ||
inline var pow(const Var& base, Arith exponent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with merging as is. Should we make a separate issue to have all these be passed by value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should definitely go in a separate issue, along with performance evaluations.
Summary
This pulls out / cleans up some of the stuff in the complex branch. Namely
var
operatorsstd::iterator_traits
specializations forvar
andfvar
Tests
@bob-carpenter do need additional tests for the iterators?
Side Effects
std will have `std::iterator_trait specializations for var and fvar
Checklist
Math issue complex numbers with vars #123
Copyright holder: Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested